-
Notifications
You must be signed in to change notification settings - Fork 254
Improvement/cldsrv 724-backbeat-related-functional-tests #5985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvement/cldsrv 724-backbeat-related-functional-tests #5985
Conversation
72dd6ed to
cd35cca
Compare
425731c to
2b9959d
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (33.33%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
... and 28 files with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #5985 +/- ##
===================================================
+ Coverage 81.00% 84.35% +3.34%
===================================================
Files 204 204
Lines 12890 12899 +9
===================================================
+ Hits 10442 10881 +439
+ Misses 2448 2018 -430
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
8c9eb10 to
3ab4eae
Compare
17d2fcb to
198582c
Compare
2c806e4 to
836c556
Compare
26ce909 to
8383cf9
Compare
cd35cca to
0fec989
Compare
DarkIsDude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats again. Sylvain did a good review, not a lot to add more
.github/workflows/tests.yaml
Outdated
| password: ${{ secrets.ARTIFACTS_PASSWORD }} | ||
| source: /tmp/artifacts | ||
| if: always() | ||
| # ceph-backend-test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just delete them ? We have githistory if needed
lib/data/wrapper.js
Outdated
| client = new DataFileInterface(config); | ||
| implName = 'file'; | ||
| } else if (config.backends.data === 'multiple') { | ||
| Object.keys(config.locationConstraints).filter(k => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree too, it's weird ?
| const expectedLocation = data.LocationConstraint || ''; | ||
| assert.deepStrictEqual(expectedLocation, ''); | ||
| // SDK v3 returns undefined for us-east-1, normalize to empty string for comparison | ||
| const locationConstraint = data.LocationConstraint || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then if I get it right we can check that the value is undefined directly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up ?
| done(); | ||
| } | ||
| } else if (testResult && typeof testResult === 'object' && testResult.code) { | ||
| // This was expected to be an error, but we got success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // This was expected to be an error, but we got success |
tests/functional/aws-node-sdk/test/multipleBackend/acl/aclAwsVersioning.js
Show resolved
Hide resolved
| 'backend successfully', done => { | ||
| const key = `somekey-${genUniqID()}`; | ||
| async.waterfall([ | ||
| next => waitForVersioningBeforePut(s3, bucket, err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here and following iteration
| next => { | ||
| s3.send(new AbortMultipartUploadCommand(paramsAzure)) | ||
| .then(() => next()) | ||
| .catch(next); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| next => { | |
| s3.send(new AbortMultipartUploadCommand(paramsAzure)) | |
| .then(() => next()) | |
| .catch(next); | |
| }, | |
| async() => s3.send(new AbortMultipartUploadCommand(paramsAzure)), |
we can do that ?
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| })) | ||
| .then(() => next()) | ||
| .catch(next), | ||
| next => s3.send(new DeleteObjectCommand({ Bucket: testBucket, Key: keyName1 })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too (and it's not the only one)
| next => s3.send(new DeleteObjectCommand({ Bucket: testBucket, Key: keyName1 })) | |
| async () => s3.send(new DeleteObjectCommand({ Bucket: testBucket, Key: keyName1 })), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep the next here it's part of a chain
3a0af19 to
7e3a81d
Compare
94d9c72 to
ce181cb
Compare
DarkIsDude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of them are nit. That's why it's approved. You can resolve them and merge or reassign me.
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.2.41", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#d1de8c4ac819ee7363323fa8ab14fcdba37a5e65", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump arsenal, reminder for you
| const expectedLocation = data.LocationConstraint || ''; | ||
| assert.deepStrictEqual(expectedLocation, ''); | ||
| // SDK v3 returns undefined for us-east-1, normalize to empty string for comparison | ||
| const locationConstraint = data.LocationConstraint || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up ?
| after(() => { | ||
| process.stdout.write('Deleting bucket\n'); | ||
| return bucketUtil.deleteOne(bucketName) | ||
| .catch(err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent 🙏
| const data = await s3.send(new GetBucketLocationCommand({ Bucket: bucketName })); | ||
| assert.strictEqual(data.LocationConstraint, endpoint); | ||
|
|
||
| // S3C backend has 'dc-1' as default location constraint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code is pretty clean alone ?
| utils.enableVersioning = async (s3, bucket) => { | ||
| await s3.send(new PutBucketVersioningCommand({ | ||
| utils.enableVersioning = (s3, bucket, callback) => { | ||
| // Support both callback and promise patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Support both callback and promise patterns |
That's not needed
| try { | ||
| const results = []; | ||
| for (const data of dataArray) { | ||
| // Don't pass callback to putToAwsBackend - we need the version ID from the promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Don't pass callback to putToAwsBackend - we need the version ID from the promise |
|
|
||
| return fromIni({ profile, filepath: filename }); | ||
| // Parse the INI file manually for synchronous access | ||
| const content = fs.readFileSync(filename, 'utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, do we really need that ? Why this change and not using the SDK or access the SDK config ?
| MultipartUpload: { Parts: parts }, | ||
| }; | ||
| gcpClient.completeMultipartUpload(params, (err, res) => { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed or the linter rule should be updated 🙏 ?
|
|
||
| it('should successfully complete MPU', | ||
| function testFn(done) { | ||
| this.timeout(1200000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explain the why here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, ci passing without it
SylvainSenechal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last comments addressed in another pr
Please note that this PR also takles the rest of the unmigrated code , better to review it by commit to have the context
Issue: CLDSRV-724